-
Notifications
You must be signed in to change notification settings - Fork 3.9k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat(js): handling the web socket connection and events #5704
Conversation
❌ Deploy Preview for novu-design failed. Why did it fail? →
|
@@ -1,4 +1,4 @@ | |||
import { | |||
import type { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
use import type
everywhere in the @novu/client
so that @novu/shared
won't be included in the bundle
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
💭 thought (non-blocking): If we are just using the types, maybe we can move the @novu/shared
under the devDependency
as well.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We can't because of the types generated for ESM, they are not "copied" during the build time.
protected onSessionSuccess(_: Session): void {} | ||
|
||
protected onSessionError(_: unknown): void {} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These two functions will be overridden by the class that extends this one.
@@ -25,7 +25,7 @@ export class NovuEventEmitter { | |||
} | |||
|
|||
off<Key extends EventNames>(eventName: Key, listener: EventHandler<Events[Key]>): void { | |||
this.#mittEmitter.on(eventName, listener); | |||
this.#mittEmitter.off(eventName, listener); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
my fault 😅
if (this.#socket.isSocketEvent(eventName)) { | ||
this.#socket.initialize(); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
When someone tries to listen to one of the WS events then initialize the WebSocket connection and add listeners on events.
packages/js/src/socket/socket.ts
Outdated
#notificationReceived = ({ message }: { message: TODO }) => { | ||
this.#emitter.emit(NOTIFICATION_RECEIVED, { | ||
result: new Notification(message), | ||
}); | ||
}; | ||
|
||
#unseenCountChanged = ({ unseenCount }: { unseenCount: number }) => { | ||
this.#emitter.emit(UNSEEN_COUNT_CHANGED, { | ||
result: unseenCount, | ||
}); | ||
}; | ||
|
||
#unreadCountChanged = ({ unreadCount }: { unreadCount: number }) => { | ||
this.#emitter.emit(UNREAD_COUNT_CHANGED, { | ||
result: unreadCount, | ||
}); | ||
}; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Handle different types of WebSocket events and just call our emitter.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What type of information do we get from WS currently, except unread_count? I'd like to suggest to have one updated event for all WS signals for easier scalability in the future.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
you can check the three above functions and their interfaces, that's what is called and returned
packages/js/src/socket/socket.ts
Outdated
this.#socketIo = io(this.#socketUrl, { | ||
reconnectionDelayMax: 10000, | ||
transports: ['websocket'], | ||
query: { | ||
token: `${this.#token}`, | ||
}, | ||
}); | ||
|
||
this.#socketIo.on('connect', () => { | ||
this.#emitter.emit('socket.connect.success', { args, result: undefined }); | ||
}); | ||
|
||
this.#socketIo.on('connect_error', (error) => { | ||
this.#emitter.emit('socket.connect.error', { args, error }); | ||
}); | ||
|
||
this.#socketIo?.on(WebSocketEvent.RECEIVED, this.#notificationReceived); | ||
this.#socketIo?.on(WebSocketEvent.UNSEEN, this.#unseenCountChanged); | ||
this.#socketIo?.on(WebSocketEvent.UNREAD, this.#unreadCountChanged); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Create the socket connection and listen on the events.
packages/js/src/socket/socket.ts
Outdated
if (this.#token) { | ||
this.#initializeSocket().then((error) => console.error(error)); | ||
|
||
return; | ||
} | ||
|
||
this.callWithSession(async () => { | ||
this.#initializeSocket().then((error) => console.error(error)); | ||
}); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
When there is a session just initialize the socket, otherwise, initialize it when the session is created (lazily).
packages/js/tsup.config.ts
Outdated
// { | ||
// ...baseConfig, | ||
// entry: { novu: 'src/umd.ts' }, | ||
// format: ['iife'], | ||
// minify: true, | ||
// dts: false, | ||
// outExtension: () => { | ||
// return { | ||
// js: '.min.js', | ||
// }; | ||
// }, | ||
// esbuildPlugins: [ | ||
// compress({ | ||
// gzip: true, | ||
// brotli: false, | ||
// outputDir: '.', | ||
// exclude: ['**/*.map'], | ||
// }), | ||
// ], | ||
// }, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Commented out this code temporarily until we investigate why Rollup includes in UMD build some additional code that results in bigger bundle size.
713d403
to
708b237
Compare
d7cadeb
to
3a0e758
Compare
3a0e758
to
4a5ef31
Compare
b57c777
to
9b0473d
Compare
4a5ef31
to
99896b5
Compare
* feat: ui solid * revert: novu/js package * feat: implement solidjs * refactor: update file name * fix: umd build * refactor: remove types from config * refactor: update the require ext * refactor: remove unused deps * refactor: rename app to inbox * feat: use novuoptions type * deat: update tsconfig * feat: remove rootelement on dispose * refactor: rename ui to inboxui * feat: add ui to umd * feat: update tsup build and removed umd for ui * refactor: imports and babel * chore: remove comment
4bbd103
to
bc2510d
Compare
✅ Deploy Preview for dev-web-novu ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
* feat(js): js sdk feeds module (#5688) * feat(js): improve the package json exports and tsup config * feat(js): lazy session initialization and interface fixes * feat(js): js sdk feeds module * feat(js): js sdk preferences (#5701) * feat(js): js sdk preferences * feat(js): handling the web socket connection and events (#5704) * feat(js): handling the web socket connection and events * feat: ui solid --------- Co-authored-by: Biswajeet Das <[email protected]> * fix: caching for session initialize * fix: worker --------- Co-authored-by: Paweł Tymczuk <[email protected]> Co-authored-by: Biswajeet Das <[email protected]> Co-authored-by: Dima Grossman <[email protected]>
What changed? Why was the change needed?
The changes:
Screenshots
Expand for optional sections
Related enterprise PR
Special notes for your reviewer